Enable array buffers in JSCRuntime.cpp #28961

ryantrem · 2020-05-25T01:10:27Z

Summary

The JavaScriptCore implementation of JSI does not currently support array buffers. The comments in the code suggest the JSC version used by React Native does not work with array buffers, but this seems to be out of date since the current version of JSC used by React Native does indeed support array buffers. This change just enables array buffers in JSCRuntime.cpp.

NOTE: See react-native-community/discussions-and-proposals#91 (comment) for more background on this change.

Changelog

[General] [Added] - Support for array buffers in the JavaScriptCore implementation of JSI

Test Plan

To test these changes, I just made some temporary changes to RNTester to use JSI to inject a test function into the JS runtime that reads from and writes to an array buffer, and call that function from the JS of the RNTester app (see ryantrem@28152ce).

For the JS side of this, specifically look at https://github.com/ryantrem/react-native/blob/28152ce3f4ae0fa906557415d106399b3f072118/RNTester/js/RNTesterApp.android.js#L13-L18

For the native side of this, specifically look at https://github.com/ryantrem/react-native/blob/28152ce3f4ae0fa906557415d106399b3f072118/RNTester/android/app/src/main/cpp/JSITest.cpp#L22-L38

…ve supports them

ryantrem · 2020-05-25T01:11:52Z

FYI @mhorowitz, @tmikov, @bghgary.

analysis-bot · 2020-05-25T01:37:24Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	10,738,000	16,304

Base commit: f438a6e

analysis-bot · 2020-05-25T01:44:07Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	6,780,258	15,628
android	hermes	armeabi-v7a	6,442,477	16,108
android	hermes	x86	7,165,449	16,074
android	hermes	x86_64	7,056,016	16,084
android	jsc	arm64-v8a	8,948,058	14,971
android	jsc	armeabi-v7a	8,602,657	15,439
android	jsc	x86	8,776,347	15,405
android	jsc	x86_64	9,352,558	15,417

Base commit: f438a6e

facebook-github-bot

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ReactCommon/jsi/JSCRuntime.cpp

…uffer related JSC functions

ryantrem · 2020-05-26T23:56:13Z

ReactCommon/jsi/JSCRuntime.cpp

@@ -276,6 +276,9 @@ class JSCRuntime : public jsi::Runtime {
 #if __IPHONE_OS_VERSION_MIN_REQUIRED >= __IPHONE_9_0


Worth noting, this existing define looks wrong to me. I believe _JSC_FAST_IS_ARRAY will never be defined on Android, so will always use the slow version. Presumably this should be fixed, though I didn't really want to overload this PR.

facebook-github-bot

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ryantrem · 2020-05-27T18:09:59Z

From what I can tell, there is no way for me to see what the warning is that is causing the "Facebook Internal - Builds & Tests" check to fail, is that right? Assuming yes, can someone (@shergin?) provide some details if there is something more I need to address for this?

ryantrem · 2020-05-29T00:31:25Z

Ping! @shergin, @mhorowitz, @tmikov - can anyone help here? I'd like to see this PR merged, but I don't know what the build warning is. I don't get any build warnings in either XCode or Android Studio when building locally.

tmikov · 2020-05-29T00:35:41Z

AFAICT, it is a clang-format warning on line 944. The line is too long and needs to be reformatted.

ryantrem · 2020-06-01T18:15:03Z

@shergin or @tmikov - thanks for your help so far on this PR. Can one of you re-"import" this PR after my latest change (which I believe should address the issue @tmikov mentioned above)?

tmikov · 2020-06-02T01:15:52Z

@ryantrem the original importer "owns" the diff internally, so ordinarily only they can re-import it. I will reach out to @shergin internally.

ryantrem · 2020-06-08T16:07:12Z

Hi again @tmikov and @shergin - sorry to nag, but since another week has passed I wanted to follow up on this and see if there has been any progress, or if you need anything else from me.

facebook-github-bot

@tmikov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

tmikov · 2020-06-08T19:20:35Z

@ryantrem sorry for the delay. We discussed it with @shergin and we will proceed to land it shortly.

react-native-bot · 2020-06-09T08:15:06Z

This pull request was successfully merged by @ryantrem in 9c32140.

^{When will my fix make it into a release? | Upcoming Releases}

Summary: The JavaScriptCore implementation of JSI [does not currently support array buffers](https://github.com/facebook/react-native/blob/master/ReactCommon/jsi/JSCRuntime.cpp#L925-L943). The comments in the code suggest the JSC version used by React Native does not work with array buffers, but this seems to be out of date since the current version of JSC used by React Native does indeed support array buffers. This change just enables array buffers in JSCRuntime.cpp. NOTE: See react-native-community/discussions-and-proposals#91 (comment) for more background on this change. ## Changelog [General] [Added] - Support for array buffers in the JavaScriptCore implementation of JSI Pull Request resolved: #28961 Test Plan: To test these changes, I just made some temporary changes to RNTester to use JSI to inject a test function into the JS runtime that reads from and writes to an array buffer, and call that function from the JS of the RNTester app (see ryantrem@28152ce). For the JS side of this, specifically look at https://github.com/ryantrem/react-native/blob/28152ce3f4ae0fa906557415d106399b3f072118/RNTester/js/RNTesterApp.android.js#L13-L18 For the native side of this, specifically look at https://github.com/ryantrem/react-native/blob/28152ce3f4ae0fa906557415d106399b3f072118/RNTester/android/app/src/main/cpp/JSITest.cpp#L22-L38 Reviewed By: shergin Differential Revision: D21717995 Pulled By: tmikov fbshipit-source-id: 5788479bb33c24d01aa80fa7f509e0ff9dcefea6

Enable array buffers since the current JSC version used in react nati…

98de687

…ve supports them

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 25, 2020

react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label May 25, 2020

ryantrem changed the title ~~Enable array buffers since the current JSC version used in react nati…~~ Enable array buffers in JSCRuntime.cpp May 25, 2020

facebook-github-bot reviewed May 26, 2020

View reviewed changes

shergin suggested changes May 26, 2020

View reviewed changes

ReactCommon/jsi/JSCRuntime.cpp Show resolved Hide resolved

Add compile time checks for iOS/MacOS versions before calling array b…

be548fd

…uffer related JSC functions

ryantrem commented May 26, 2020

View reviewed changes

ryantrem requested a review from shergin May 27, 2020 03:25

facebook-github-bot reviewed May 27, 2020

View reviewed changes

Restore original formatting to not break linting(?) rules

e002766

ryantrem mentioned this pull request Jun 8, 2020

Push through support for array buffers in React Native's JSCRuntime BabylonJS/BabylonNative#288

Closed

facebook-github-bot reviewed Jun 8, 2020

View reviewed changes

facebook-github-bot closed this in 9c32140 Jun 9, 2020

react-native-bot added the Merged This PR has been merged. label Jun 9, 2020

ryantrem mentioned this pull request Jun 9, 2020

JSI (JavaScript Interface) & JSC (JavaScript Core) Discussion react-native-community/discussions-and-proposals#91

Closed

wkozyra95 mentioned this pull request Jun 19, 2020

enable ArrayBuffer API in JSCRuntime #28634

Closed

savv mentioned this pull request Nov 21, 2020

Add methods to create ArrayBuffers from byte arrays #30445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable array buffers in JSCRuntime.cpp #28961

Enable array buffers in JSCRuntime.cpp #28961

ryantrem commented May 25, 2020 •

edited

Loading

ryantrem commented May 25, 2020

analysis-bot commented May 25, 2020 •

edited

Loading

analysis-bot commented May 25, 2020 •

edited

Loading

facebook-github-bot left a comment

ryantrem May 26, 2020

facebook-github-bot left a comment

ryantrem commented May 27, 2020

ryantrem commented May 29, 2020

tmikov commented May 29, 2020 •

edited

Loading

ryantrem commented Jun 1, 2020

tmikov commented Jun 2, 2020 •

edited

Loading

ryantrem commented Jun 8, 2020

facebook-github-bot left a comment

tmikov commented Jun 8, 2020

react-native-bot commented Jun 9, 2020

		@@ -276,6 +276,9 @@ class JSCRuntime : public jsi::Runtime {
		#if __IPHONE_OS_VERSION_MIN_REQUIRED >= __IPHONE_9_0

Enable array buffers in JSCRuntime.cpp #28961

Enable array buffers in JSCRuntime.cpp #28961

Conversation

ryantrem commented May 25, 2020 • edited Loading

Summary

Changelog

Test Plan

ryantrem commented May 25, 2020

analysis-bot commented May 25, 2020 • edited Loading

analysis-bot commented May 25, 2020 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

ryantrem May 26, 2020

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

ryantrem commented May 27, 2020

ryantrem commented May 29, 2020

tmikov commented May 29, 2020 • edited Loading

ryantrem commented Jun 1, 2020

tmikov commented Jun 2, 2020 • edited Loading

ryantrem commented Jun 8, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

tmikov commented Jun 8, 2020

react-native-bot commented Jun 9, 2020

ryantrem commented May 25, 2020 •

edited

Loading

analysis-bot commented May 25, 2020 •

edited

Loading

analysis-bot commented May 25, 2020 •

edited

Loading

tmikov commented May 29, 2020 •

edited

Loading

tmikov commented Jun 2, 2020 •

edited

Loading